Issue #3395: Fix tool table formatting error with random tool changer #3428
Conversation
| } else { | ||
| snprintf(tmp,sizeof(tmp),"\n"); | ||
| } | ||
| strncat(formatted_line,tmp,CANON_TOOL_ENTRY_LEN-1); |
There was a problem hiding this comment.
strncat "appends at most ssize non-null bytes from the array pointed to by src". It does not check how much is already written to the string or how large the buffer truly is. So, the length of formatted_line in my understanding needs to be substracted from CANON_TOOL_ENTRY_LEN-1 to be safe.
This is all a bit cumbersome. I suggest to not execute the strncat in line 241 and instead keep the value "tmp" intact by not overwriting it in line 273. Instead, in line 273 perform the snprintf into formatted_line and check the number of characters written. Like this:
new line 273:
int charsThatWouldBeWritten snprintf(formatted_line,CANON_TOOL_ENTRY_LEN-1,"%s ;%s\n", tmp, tdata.comment);
if (charsThatWouldBeWritten<0) {
fprintf(stderr,"Could not be printed. No good. Maybe this computer should not control any machine.\n");-1
if (! charsThatWouldBeWritten<CANON_TOOL_ENTRY_LEN) {
fprintf(stderr,"Ouch! May still be ok if the bits up the comment were copied. Not checked.\n");
}
and analogous for the part just attaching the "\n".
This would somewhat guarantee that all non-comment bits are truly read - This may avoid some weird situation to have some tool's length shortened by a couple of decimals positions and nobody notices that. Admittedly, after a quick check on http://wiki.linuxcnc.org/cgi-bin/wiki.pl?ToolTable it seems unlikely to create such a long line.
The code does not check if strlen(tdata.comment) < CANON_TOOL_COMMENT_SIZE . This would allow the routine to create formatted lines that do not follow the constraints set by the format.
There was a problem hiding this comment.
This seems wrong?
char tmp[CANON_TOOL_ENTRY_LEN-1] = {0};
Shouldn't tmp be the full size, then we should use sizeof-1 to leave space for the null?
As for the rest of it, I wonder if this works?
char tmp[CANON_TOOL_ENTRY_LEN-1] = {0};
int space = sizeof(tmp);
int len;
len= snprintf(tmp,sizeof(tmp),"T%-3d P%-3d"
,tdata.toolno
,is_random_toolchanger ? idx : tdata.pocketno);
strncat(formatted_line,tmp,space);
space -= len;
// format zero float values as %.0f for brevity
#define F_ITEM(item,letter) if (!ignore_zero_values || tdata.item) { \
if (tdata.item) { \
len -= snprintf(tmp,sizeof(tmp)," " letter "%+f", tdata.item); \
} else { \
len -= snprintf(tmp,sizeof(tmp)," " letter "%.0f",tdata.item); \
} \
strncat(formatted_line,tmp,space; \
space -= len;
}
#define I_ITEM(item,letter) if (!ignore_zero_values || tdata.item) { \
len = snprintf(tmp,sizeof(tmp)," " letter "%d",tdata.item); \
strncat(formatted_line,tmp,space); \
space -= len;
}
F_ITEM(diameter, "D");
F_ITEM(offset.tran.x, "X");
F_ITEM(offset.tran.y, "Y");
F_ITEM(offset.tran.z, "Z");
F_ITEM(offset.a, "A");
F_ITEM(offset.b, "B");
F_ITEM(offset.c, "C");
F_ITEM(offset.u, "U");
F_ITEM(offset.v, "V");
F_ITEM(offset.w, "W");
F_ITEM(frontangle, "I");
F_ITEM(backangle, "J");
I_ITEM(orientation, "Q");
#undef F_ITEM
#undef I_ITEM
// Stop tracking len and space here for current tool table format
if (tdata.comment[0]) {
snprintf(tmp,space," ;%s\n",tdata.comment);
} else {
snprintf(tmp,space,"\n");
}
strncat(formatted_line,tmp,space);
Issue introduced in 088be31